Look for unmached brace count when parsing generic at-rules #156
Conversation
tests/Sabberworm/CSS/ParserTest.php
Outdated
| @@ -420,6 +420,12 @@ function testEmptyGridLineNameLenientInFile() { | |||
| $this->assertSame($sExpected, $oDoc->render()); | |||
| } | |||
|
|
|||
| function testUnmachedBracesInFile() { | |||
| $oDoc = $this->parsedStructureForFile('unmached_braces', Settings::create()->withMultibyteSupport(true)); | |||
There was a problem hiding this comment.
Typo in file name unmatched_braces…
There was a problem hiding this comment.
Lolz, not only in the file name 😅
| @@ -142,6 +142,13 @@ private static function parseAtRule(ParserState $oParserState) { | |||
| } else { | |||
| //Unknown other at rule (font-face or such) | |||
| $sArgs = trim($oParserState->consumeUntil('{', false, true)); | |||
| if (substr_count($sArgs, "(") != substr_count($sArgs, ")")) { | |||
| if($oParserState->getSettings()->bLenientParsing) { | |||
| return NULL; | |||
There was a problem hiding this comment.
Will this mean there’s a null item in the resulting document’s contents? Or does the calling code ignore returned nulls?
There was a problem hiding this comment.
There are no null elements in the resulting document. The calling code looks like this:
$oListItem = null;
if($bLenientParsing) {
try {
$oListItem = self::parseListItem($oParserState, $oList);
} catch (UnexpectedTokenException $e) {
$oListItem = false;
}
} else {
$oListItem = self::parseListItem($oParserState, $oList);
}
if($oListItem === null) {
// List parsing finished
return;
}Returning null aborts further parsing, which is the desired behavior. Otherwise the resulting CSS will (most likely) be valid which will change the final rendering of pages with this issue.
There was a problem hiding this comment.
Ok, I get it now, thanks…
There was a problem hiding this comment.
I presume you meant “resulting CSS will (most likely) be _in_valid”
There was a problem hiding this comment.
I meant "valid", in the sense that the broken rules will be discarded, but the rest of the rules, which I presume are valid, will be included in the file.
However, this will be an issue, since those rules have been previously ignored by the browsers, so including them will most likely alter the final rendering of the page.
| @@ -142,6 +142,13 @@ private static function parseAtRule(ParserState $oParserState) { | |||
| } else { | |||
| //Unknown other at rule (font-face or such) | |||
| $sArgs = trim($oParserState->consumeUntil('{', false, true)); | |||
| if (substr_count($sArgs, "(") != substr_count($sArgs, ")")) { | |||
| if($oParserState->getSettings()->bLenientParsing) { | |||
There was a problem hiding this comment.
Do we need this if? Can’t we just throw an UnsupportedTokenException instead of a SourceException and the calling code will catch it in lenient mode? If that’s not the case, maybe it should be…
There was a problem hiding this comment.
Throwing an UnsupportedTokenException will not abort parsing, which leads to other kind of issues.
Browsers seem to stop CSS parsing on unmatched braces count for the at-rules. I guess we should do the same.